Skip to content

fix(rebuild,mcp): surface errors and recover prepared destroys - #6377

Merged
apurvvkumaria merged 10 commits into
mainfrom
fix/mcp-destroy-marker-silent-rebuild-6376
Jul 8, 2026
Merged

fix(rebuild,mcp): surface errors and recover prepared destroys#6377
apurvvkumaria merged 10 commits into
mainfrom
fix/mcp-destroy-marker-silent-rebuild-6376

Conversation

@yanyunl1991

@yanyunl1991 yanyunl1991 commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR makes rebuild surface redacted MCP destroy diagnostics before backup or deletion and adds a non-destructive recovery path for prepared-only destroy transactions.

It deliberately keeps pending or both-marker transactions fail closed because destroyPendingAt records confirmed OpenShell deletion while provider or policy cleanup may still be owed.

Related Issue

Addresses #6376.

The issue's literal reproduction manually injects both markers into a live sandbox. That is not a valid lifecycle transition. When destroyPendingAt is present, pending takes precedence and recovery must finish through nemoclaw <name> destroy rather than erase durable retry state.

Recovery Contract

Registry state Meaning Recovery
No destroy marker No durable destroy transaction is active. Normal MCP commands and rebuild proceed.
destroyPreparedAt only Adapter scrub and provider detach completed, but sandbox deletion is not durably confirmed. If the sandbox is still live, run nemoclaw <name> mcp remove <server> --force for each registered server. The marker clears only after residual-free cleanup drains every bridge entry.
destroyPendingAt, with or without destroyPreparedAt The registry records confirmed OpenShell deletion and retained cleanup may still be owed. mcp remove --force refuses and preserves both markers and the cleanup manifest. Run nemoclaw <name> destroy to finish idempotent cleanup.

Changes

  • Print non-throw rebuild bail messages on stderr through the existing redaction boundary.
  • Check MCP destroy markers during rebuild preflight, before target preflights, backup, or deletion.
  • Clear a prepared marker only after the requested removal succeeds without residuals and no bridge entries remain.
  • Preserve markers and manifests after failures, wrong-server no-ops, tolerated residuals, partial multi-bridge cleanup, and pending or both-marker refusal.
  • Assert that destroy-abort and rebuild restoration do not rotate the ambient host credential value.
  • Document the phase-aware recovery contract in the MCP guide and command references.

Type of Change

  • Code change (feature, bug fix, or refactor)
  • Code change with doc updates
  • Doc only (prose changes, no code sample modifications)
  • Doc only (includes code sample changes)

Quality Gates

  • Tests added or updated for changed behavior
  • Existing tests cover changed behavior — justification:
  • Tests not applicable — justification:
  • Docs updated for user-facing behavior changes
  • Docs not applicable — justification:
  • Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging)
  • Sensitive-path review completed or maintainer-approved waiver recorded — marker ordering, pending cleanup ownership, failure preservation, and the prepared-only recovery boundary were reviewed against the durable state transitions and locked with regression tests.
  • Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Verification

  • PR description includes the DCO sign-off declaration and every commit appears as Verified in GitHub
  • Normal pre-commit, commit-msg, and pre-push hooks passed
  • Targeted behavior tests pass for the current change set:
    • npx vitest run --project integration test/mcp-bridge-destroy-marker-recovery.test.ts test/mcp-destroy-lifecycle.test.ts — 32/32
    • npx vitest run --project cli src/lib/actions/sandbox/mcp-bridge-*.test.ts — 106/106
  • Applicable broad gate passed — not run because the change is scoped to MCP lifecycle behavior; the full MCP CLI scope and focused integration suite passed.
  • Quality Gates section completed with required justifications
  • No secrets, API keys, or credentials committed
  • npm run docs builds without warnings — completed with 0 errors and 2 existing Fern warnings
  • Doc pages follow the style guide
  • New doc pages include SPDX header and frontmatter — no new pages

Additional checks:

  • npm run typecheck:cli
  • npm run test:titles:check
  • npm run test-size:check
  • Biome check on all touched TypeScript and test files
  • Generated agent-variant documentation consistency check

AI Disclosure

  • AI-assisted — tools: Claude Code and Codex

Signed-off-by: Yanyun Liao yanyunl@nvidia.com
Signed-off-by: Apurv Kumaria akumaria@nvidia.com

…covery

Two related defects reported together (#6376):

FACET A — rebuild aborts silently on a stuck MCP destroy transaction.
`createRebuildCommandContext`'s non-throw bail was
`(_message, code = 1) => process.exit(code)` — the message argument was
discarded. Every `bail(msg, code)` call in the rebuild pipeline
therefore exited without printing anything, so an actionable reason
(e.g. "Failed to preserve MCP bridges before rebuild: Sandbox 'X' has
an incomplete MCP destroy transaction. Re-run the sandbox destroy
command …") produced `Deleting old sandbox... EXIT:1` with no
diagnosis at all. Emit the message on stderr with the two-space
rebuild-diagnostic prefix before exit, so `$?`-gated automation and
interactive users see WHY rebuild aborted. Empty messages still exit
silently to stay backward-compatible.

FACET B — no non-destructive recovery for the stuck state. `mcp remove
<server> --force` ran `assertMcpDestroyNotPending` before its `--force`
branch, so the same guard fired and the only advertised recovery was
`nemoclaw <sandbox> destroy` (full sandbox destruction). Add a new
`clearMcpDestroyMarkers(sandboxName)` helper in `mcp-bridge-state.ts`
that clears the `destroyPreparedAt` / `destroyPendingAt` markers in
place without touching bridge state, provider records, adapters, or
policy — the marker file is the only mutable state the guard reads.
Wire it into `removeMcpBridgeUnlocked`: when `--force` is set and
markers are present, clear them, log the recovery, re-fetch the
sandbox, then run the normal removal flow (which may now find the
server already gone and no-op, or continue with the actual removal).
Also update the guard's error text to name the new recovery path so
users can find it without reading the source.

Regression coverage:
- `src/lib/actions/sandbox/rebuild-preflight-confirmation.test.ts`
  (3 new cases) — non-throw bail prints the message before exit,
  empty-message backward-compat, throw-on-error path stays lossless.
- `test/mcp-bridge-destroy-marker-recovery.test.ts` (new file, 5 cases)
  — `clearMcpDestroyMarkers` clears both markers, is a no-op when
  none set, handles either marker set alone;
  `removeMcpBridge(--force)` clears markers end-to-end and logs;
  `removeMcpBridge` WITHOUT `--force` still refuses (safety unchanged).

Fixes #6376

Signed-off-by: Yanyun Liao <yanyunl@nvidia.com>
@coderabbitai

coderabbitai Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

This PR adds phase-aware MCP destroy marker recovery, enables mcp remove --force to clear prepared-only destroy state after success, makes rebuild fail fast with an error message when destroy markers exist, and expands regression test coverage.

Changes

MCP Destroy Marker Recovery

Layer / File(s) Summary
State helper and guard message
src/lib/actions/sandbox/mcp-bridge-state.ts
Updates the destroy guard to distinguish prepared and pending markers, and adds a helper that clears only the prepared marker while preserving other MCP state.
Forced remove recovery path
src/lib/actions/sandbox/mcp-bridge-remove.ts
Imports the marker-clearing helper, snapshots MCP state before removal, clears recoverable prepared-destroy markers after successful forced removal, and bypasses the guard only for that recoverable phase.
Rebuild bail output
src/lib/actions/sandbox/rebuild-preflight-confirmation.ts, src/lib/actions/sandbox/rebuild-preflight-phase.ts, src/lib/actions/sandbox/rebuild-preflight-confirmation.test.ts, test/mcp-destroy-lifecycle.test.ts
Changes rebuild preflight bail handling to emit the error message before exiting and to stop later rebuild work immediately when destroy markers are detected. Tests cover stderr output, redaction, and phase-aware guard messages.
Recovery regression tests
test/mcp-bridge-destroy-marker-recovery.test.ts
Adds direct coverage for prepared-marker clearing, pending-marker refusal, forced recovery outcomes, rebuild guard behavior, and non-forced recovery guidance.

Estimated code review effort: 4 (Complex) | ~45 minutes

Suggested labels: bug-fix

Suggested reviewers: jyaunches, cjagwani, ericksoa

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The PR addresses #6376, but it does not implement the unrelated direct requirement in #39 to add the hindsight-memory skill files and docs. Either add the hindsight-memory skill package and its docs for #39, or remove #39 from the linked issues if it was attached by mistake.
✅ Passed checks (4 passed)
Check name Status Explanation
Out of Scope Changes check ✅ Passed The changed files stay focused on rebuild diagnostics and MCP destroy recovery, with no unrelated code paths added.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the two main fixes: rebuild now surfaces bail errors and forced MCP removal can recover prepared destroys.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/mcp-destroy-marker-silent-rebuild-6376

Comment @coderabbitai help to get the list of available commands.

@github-code-quality

github-code-quality Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in the fix/mcp-destroy-mark... branch is 96%. Coverage data for the main branch is not yet available.

Show a code coverage summary of the most covered files.
File main fix/mcp-destroy-mark... d1881e0 +/-
nemoclaw/src/se...cret-scanner.ts 100%
nemoclaw/src/commands/slash.ts 100%
nemoclaw/src/li...bprocess-env.ts 100%
nemoclaw/src/bl...eprint/state.ts 98%
nemoclaw/src/onboard/config.ts 98%
nemoclaw/src/bl...int/snapshot.ts 97%
nemoclaw/src/bl...print/runner.ts 95%
nemoclaw/src/co...ration-state.ts 94%
nemoclaw/src/bl...ate-networks.ts 94%
nemoclaw/src/index.ts 94%

TypeScript / code-coverage/cli

The overall coverage in the fix/mcp-destroy-mark... branch is 76%. Coverage data for the main branch is not yet available.

Show a code coverage summary of the most covered files.
File main fix/mcp-destroy-mark... d1881e0 +/-
src/lib/onboard/preflight.ts 82%
src/lib/actions...all/run-plan.ts 81%
src/lib/state/o...oard-session.ts 81%
src/lib/actions...licy-channel.ts 79%
src/lib/state/sandbox.ts 75%
src/lib/actions...dbox/connect.ts 72%
src/lib/onboard...er-gpu-patch.ts 69%
src/lib/policy/index.ts 66%
src/lib/shields/index.ts 61%
src/lib/onboard.ts 28%

Updated July 07, 2026 22:23 UTC
Code Coverage is in Public Preview. Learn more and provide us with your feedback.

@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

E2E Advisor Recommendation

Required E2E: mcp-bridge, sandbox-rebuild
Optional E2E: mcp-bridge-dev, rebuild-openclaw

Dispatch hint: mcp-bridge,sandbox-rebuild

Workflow run

Full advisor summary

E2E Recommendation Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E

  • mcp-bridge (high): Required because the PR changes MCP bridge lifecycle cleanup and recovery paths that touch live OpenShell providers, sandbox provider attachments, generated MCP policy, adapter removal, credential boundaries, rebuild-with-MCP preservation, and force removal behavior. The existing mcp-bridge live job is the closest full-stack coverage for MCP add/status/restart/rebuild/remove/destroy cleanup across real sandbox agents.
  • sandbox-rebuild (high): Required because rebuild preflight code changed for all rebuilds, adding an MCP destroy-marker guard before backup/destructive phases and changing bail output. A live rebuild job should confirm normal rebuilds still pass through the modified preflight and complete the sandbox lifecycle.

Optional E2E

  • mcp-bridge-dev (high): Optional compatibility confidence against OpenShell dev artifacts for the same MCP provider/policy/adapter lifecycle touched here; useful if the change depends on or may expose OpenShell behavior drift, but not merge-blocking relative to the stable mcp-bridge lane.
  • rebuild-openclaw (high): Optional deeper rebuild coverage for OpenClaw old-base/current rebuild behavior. It provides additional confidence that the changed rebuild preflight and bail path do not regress legacy base-image rebuild flows.

New E2E recommendations

  • mcp-destroy-marker-recovery (high): The PR adds unit/runtime coverage for prepared-only vs pending MCP destroy markers, but existing live E2E does not appear to deliberately create an interrupted MCP destroy transaction and then exercise mcp remove --force, rebuild refusal before backup, and pending-state destroy completion against a real OpenShell sandbox/provider/policy stack.
    • Suggested test: Add a focused live E2E target that onboards a sandbox with an MCP bridge, injects host registry destroyPreparedAt/destroyPendingAt states or interrupts destroy at controlled hooks, verifies prepared-only mcp remove <server> --force clears the marker only after residual-free cleanup, verifies wrong-server/residual failures preserve it, verifies pending/both-marker state refuses force removal and points to destroy, and verifies rebuild refuses before backup/destructive work.

Dispatch hint

  • Workflow: .github/workflows/e2e.yaml
  • jobs input: mcp-bridge,sandbox-rebuild

@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

E2E Target Recommendation

Required E2E targets: mcp-bridge
Optional E2E targets: mcp-bridge-dev

Dispatch required E2E targets:

  • gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=mcp-bridge

Workflow run

Full E2E target advisor summary

E2E Target Advisor

Base: origin/main
Head: HEAD
Confidence: high

Required E2E targets

  • mcp-bridge: The PR changes managed MCP bridge destroy/remove recovery state and rebuild preflight guards. The e2e.yaml free-standing mcp-bridge job is the live E2E path wired to test/e2e/live/mcp-bridge.test.ts for MCP bridge lifecycle behavior, so prefer this discrete job over the registry fan-out.
    • Dispatch: gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=mcp-bridge

Optional E2E targets

  • mcp-bridge-dev: Optional adjacent MCP bridge coverage against the OpenShell dev channel; useful for compatibility confidence but not the primary stable live path.
    • Dispatch: gh workflow run e2e.yaml --ref <pr-head-ref> --field jobs=mcp-bridge-dev

Relevant changed files

  • src/lib/actions/sandbox/mcp-bridge-remove.ts
  • src/lib/actions/sandbox/mcp-bridge-state.ts
  • src/lib/actions/sandbox/rebuild-preflight-confirmation.ts
  • src/lib/actions/sandbox/rebuild-preflight-phase.ts

@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor (Nemotron Ultra) — Changes requested

Merge posture: Do not merge yet
Primary next action: Add or justify PRA-T1 and any related test follow-ups.
Open items: 0 required · 0 warnings · 4 suggestions · 4 test follow-ups
Since last review: 0 prior items resolved · 2 still apply · 1 new item found

Action checklist

  • PRA-T1 Add or justify test follow-up: Runtime validation
  • PRA-T2 Add or justify test follow-up: Runtime validation
  • PRA-T3 Add or justify test follow-up: Add multi-bridge --force recovery ordering test matching mcp-bridge-destroy.ts phase two sequence
  • PRA-T4 Add or justify test follow-up: Add concurrent mcp remove --force + rebuild race test under lifecycle lock
  • PRA-1 In-scope improvement: Monolith growth: mcp-bridge-remove.ts grew by 72 lines — extract destroy-recovery logic in src/lib/actions/sandbox/mcp-bridge-remove.ts:1
  • PRA-2 In-scope improvement: Monolith growth: mcp-bridge-state.ts grew by 86 lines — extract destroy-marker helpers in src/lib/actions/sandbox/mcp-bridge-state.ts:1
  • PRA-3 In-scope improvement: Add multi-bridge --force recovery ordering test matching mcp-bridge-destroy.ts phase two sequence in test/mcp-bridge-destroy-marker-recovery.test.ts
  • PRA-4 In-scope improvement: Add concurrent mcp remove --force + rebuild race test under lifecycle lock in test/mcp-bridge-destroy-marker-recovery.test.ts

Findings index

ID Severity Category Location Required action
PRA-1 Improvement architecture src/lib/actions/sandbox/mcp-bridge-remove.ts:1 Extract McpRemovalOutcome, PROVEN_MCP_RECOVERY_OUTCOMES, completedPreparedDestroyRecovery, and clearMcpDestroyMarkers coordination into a separate module (e.g., mcp-bridge-destroy-recovery.ts). This is a current-PR action since the logic is local to the changed code.
PRA-2 Improvement architecture src/lib/actions/sandbox/mcp-bridge-state.ts:1 Extract clearMcpDestroyMarkers and phase-aware guard helpers into mcp-bridge-destroy-markers.ts. This is a current-PR action since the new helper is local to changed code.
PRA-3 Improvement tests test/mcp-bridge-destroy-marker-recovery.test.ts Add a test that registers two bridges, runs removeMcpBridge --force for first bridge (marker must persist), then for second bridge (marker must clear), and verifies the event sequence matches destroy phase two for each.
PRA-4 Improvement tests test/mcp-bridge-destroy-marker-recovery.test.ts Add a test that spawns two concurrent operations (one mcp remove --force, one rebuild) on a sandbox with prepared marker, and verifies only one succeeds with correct marker state.
Review findings by urgency: 0 required fixes, 0 items to resolve/justify, 4 in-scope improvements

⚠️ Resolve or justify before merge

Investigate these in the current review; either fix them, explain why they are not applicable, or document the accepted risk.

  • None.

💡 In-scope improvements

These are lower-risk, not throwaway. Prefer fixing them in this PR when they are local to changed code; defer only with rationale or a linked follow-up.

PRA-1 Improvement — Monolith growth: mcp-bridge-remove.ts grew by 72 lines — extract destroy-recovery logic

  • Location: src/lib/actions/sandbox/mcp-bridge-remove.ts:1
  • Category: architecture
  • Problem: File grew from 340 to 412 lines. Added McpRemovalOutcome, PROVEN_MCP_RECOVERY_OUTCOMES, completedPreparedDestroyRecovery, and clearMcpDestroyMarkers coordination. Prior advisor (PRA-1) suggested extracting to mcp-bridge-destroy-recovery.ts.
  • Impact: Continued MCP destroy work will further increase file size, making maintenance harder and increasing cognitive load for reviewers.
  • Suggested action: Extract McpRemovalOutcome, PROVEN_MCP_RECOVERY_OUTCOMES, completedPreparedDestroyRecovery, and clearMcpDestroyMarkers coordination into a separate module (e.g., mcp-bridge-destroy-recovery.ts). This is a current-PR action since the logic is local to the changed code.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Run: wc -l src/lib/actions/sandbox/mcp-bridge-remove.ts (currently 412 lines)
  • Missing regression test: N/A — architectural suggestion, no behavior change
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Drift context flagged +72 lines with severity 'blocker' for monolith growth. The new discriminated union and helper functions are the primary growth drivers.

PRA-2 Improvement — Monolith growth: mcp-bridge-state.ts grew by 86 lines — extract destroy-marker helpers

  • Location: src/lib/actions/sandbox/mcp-bridge-state.ts:1
  • Category: architecture
  • Problem: File grew from 165 to 251 lines. Added clearMcpDestroyMarkers (90 lines with JSDoc) and phase-aware assertMcpDestroyNotPending messages. Prior advisor (PRA-2) suggested splitting bridge state vs destroy marker state.
  • Impact: Bridge state and destroy marker state are related but distinct concerns. Further lifecycle work could make this module a central bottleneck.
  • Suggested action: Extract clearMcpDestroyMarkers and phase-aware guard helpers into mcp-bridge-destroy-markers.ts. This is a current-PR action since the new helper is local to changed code.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Run: wc -l src/lib/actions/sandbox/mcp-bridge-state.ts (currently 251 lines)
  • Missing regression test: N/A — architectural suggestion, no behavior change
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Drift context flagged +86 lines. The clearMcpDestroyMarkers function (90 lines with JSDoc) and updated assertMcpDestroyNotPending are the primary growth drivers.

PRA-3 Improvement — Add multi-bridge --force recovery ordering test matching mcp-bridge-destroy.ts phase two sequence

  • Location: test/mcp-bridge-destroy-marker-recovery.test.ts
  • Category: tests
  • Problem: Current test 'clears the prepared marker after removing the final committed bridge' verifies provider detach → credential revocation → policy removal → provider delete sequence for single bridge only. No test covers the ordering when multiple bridges exist and are removed sequentially.
  • Impact: Race or ordering bugs in multi-bridge cleanup could leave partial state that preserves the marker incorrectly or clears it prematurely.
  • Suggested action: Add a test that registers two bridges, runs removeMcpBridge --force for first bridge (marker must persist), then for second bridge (marker must clear), and verifies the event sequence matches destroy phase two for each.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Run: npx vitest run --project integration test/mcp-bridge-destroy-marker-recovery.test.ts -t 'multi-bridge'
  • Missing regression test: New test: 'clears the prepared marker after removing the final bridge in multi-bridge scenario with verified phase-two ordering'
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Test 'keeps the prepared marker until every bridge entry is removed' checks marker persistence but not the per-bridge event ordering.

PRA-4 Improvement — Add concurrent mcp remove --force + rebuild race test under lifecycle lock

  • Location: test/mcp-bridge-destroy-marker-recovery.test.ts
  • Category: tests
  • Problem: Both mcp remove --force and rebuild acquire withMcpLifecycleLock. No test verifies lock serialization prevents marker clear race when both run concurrently.
  • Impact: Concurrent operations could cause the prepared marker to be cleared by one operation while the other still expects it, leaving inconsistent state.
  • Suggested action: Add a test that spawns two concurrent operations (one mcp remove --force, one rebuild) on a sandbox with prepared marker, and verifies only one succeeds with correct marker state.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Run: npx vitest run --project integration test/mcp-bridge-destroy-marker-recovery.test.ts -t 'concurrent'
  • Missing regression test: New test: 'concurrent mcp remove --force and rebuild serialize via lifecycle lock with correct marker outcome'
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Lifecycle lock exists but no regression test exercises concurrent access on a stuck-destroy sandbox.
Simplification opportunities: 2 possible cuts, net -130 lines possible

These are safe simplification checks only. Do not remove validation, security controls, data-loss prevention, or required tests.

  • PRA-1 shrink (src/lib/actions/sandbox/mcp-bridge-remove.ts:1): McpRemovalOutcome type, PROVEN_MCP_RECOVERY_OUTCOMES set, completedPreparedDestroyRecovery function, and clearMcpDestroyMarkers coordination logic from removeMcpBridge
    • Replacement: New module mcp-bridge-destroy-recovery.ts exporting the outcome type, outcome validators, and marker-clear coordination
    • Net: -50 lines
    • Safety boundary: Must preserve: phase-aware clear-only-after-proven-recovery semantics, lifecycle lock serialization, ownership proofs in assertExactMcpRemoveProvider, and test coverage for all outcome branches
  • PRA-2 shrink (src/lib/actions/sandbox/mcp-bridge-state.ts:1): clearMcpDestroyMarkers function and phase-aware assertMcpDestroyNotPending message logic
    • Replacement: New module mcp-bridge-destroy-markers.ts exporting clearMcpDestroyMarkers and phase-aware guard helpers
    • Net: -80 lines
    • Safety boundary: Must preserve: phase-aware refusal for pending/both markers, validation of sandbox name before registry mutation, setBridgeState preservation of bridges/managedServerNames, and test coverage for clear/refuse/preserve behaviors
Test follow-ups to resolve or justify

If these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.

  • PRA-T1 Runtime validation — Add multi-bridge --force recovery ordering test matching mcp-bridge-destroy.ts phase two sequence. Runtime/sandbox/infrastructure paths need behavioral runtime validation. The new test file (753 lines) runs actual module code in subprocesses with isolated HOME, exercising real registry/provider/policy/adapter code paths — satisfies runtime validation for changed behavior.
  • PRA-T2 Runtime validation — Add concurrent mcp remove --force + rebuild race test under lifecycle lock. Runtime/sandbox/infrastructure paths need behavioral runtime validation. The new test file (753 lines) runs actual module code in subprocesses with isolated HOME, exercising real registry/provider/policy/adapter code paths — satisfies runtime validation for changed behavior.
  • PRA-T3 Add multi-bridge --force recovery ordering test matching mcp-bridge-destroy.ts phase two sequence — Add a test that registers two bridges, runs removeMcpBridge --force for first bridge (marker must persist), then for second bridge (marker must clear), and verifies the event sequence matches destroy phase two for each.
  • PRA-T4 Add concurrent mcp remove --force + rebuild race test under lifecycle lock — Add a test that spawns two concurrent operations (one mcp remove --force, one rebuild) on a sandbox with prepared marker, and verifies only one succeeds with correct marker state.
Since last review details

Current findings, using the urgency labels above:

PRA-1 Improvement — Monolith growth: mcp-bridge-remove.ts grew by 72 lines — extract destroy-recovery logic

  • Location: src/lib/actions/sandbox/mcp-bridge-remove.ts:1
  • Category: architecture
  • Problem: File grew from 340 to 412 lines. Added McpRemovalOutcome, PROVEN_MCP_RECOVERY_OUTCOMES, completedPreparedDestroyRecovery, and clearMcpDestroyMarkers coordination. Prior advisor (PRA-1) suggested extracting to mcp-bridge-destroy-recovery.ts.
  • Impact: Continued MCP destroy work will further increase file size, making maintenance harder and increasing cognitive load for reviewers.
  • Suggested action: Extract McpRemovalOutcome, PROVEN_MCP_RECOVERY_OUTCOMES, completedPreparedDestroyRecovery, and clearMcpDestroyMarkers coordination into a separate module (e.g., mcp-bridge-destroy-recovery.ts). This is a current-PR action since the logic is local to the changed code.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Run: wc -l src/lib/actions/sandbox/mcp-bridge-remove.ts (currently 412 lines)
  • Missing regression test: N/A — architectural suggestion, no behavior change
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Drift context flagged +72 lines with severity 'blocker' for monolith growth. The new discriminated union and helper functions are the primary growth drivers.

PRA-2 Improvement — Monolith growth: mcp-bridge-state.ts grew by 86 lines — extract destroy-marker helpers

  • Location: src/lib/actions/sandbox/mcp-bridge-state.ts:1
  • Category: architecture
  • Problem: File grew from 165 to 251 lines. Added clearMcpDestroyMarkers (90 lines with JSDoc) and phase-aware assertMcpDestroyNotPending messages. Prior advisor (PRA-2) suggested splitting bridge state vs destroy marker state.
  • Impact: Bridge state and destroy marker state are related but distinct concerns. Further lifecycle work could make this module a central bottleneck.
  • Suggested action: Extract clearMcpDestroyMarkers and phase-aware guard helpers into mcp-bridge-destroy-markers.ts. This is a current-PR action since the new helper is local to changed code.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Run: wc -l src/lib/actions/sandbox/mcp-bridge-state.ts (currently 251 lines)
  • Missing regression test: N/A — architectural suggestion, no behavior change
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Drift context flagged +86 lines. The clearMcpDestroyMarkers function (90 lines with JSDoc) and updated assertMcpDestroyNotPending are the primary growth drivers.

PRA-3 Improvement — Add multi-bridge --force recovery ordering test matching mcp-bridge-destroy.ts phase two sequence

  • Location: test/mcp-bridge-destroy-marker-recovery.test.ts
  • Category: tests
  • Problem: Current test 'clears the prepared marker after removing the final committed bridge' verifies provider detach → credential revocation → policy removal → provider delete sequence for single bridge only. No test covers the ordering when multiple bridges exist and are removed sequentially.
  • Impact: Race or ordering bugs in multi-bridge cleanup could leave partial state that preserves the marker incorrectly or clears it prematurely.
  • Suggested action: Add a test that registers two bridges, runs removeMcpBridge --force for first bridge (marker must persist), then for second bridge (marker must clear), and verifies the event sequence matches destroy phase two for each.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Run: npx vitest run --project integration test/mcp-bridge-destroy-marker-recovery.test.ts -t 'multi-bridge'
  • Missing regression test: New test: 'clears the prepared marker after removing the final bridge in multi-bridge scenario with verified phase-two ordering'
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Test 'keeps the prepared marker until every bridge entry is removed' checks marker persistence but not the per-bridge event ordering.

PRA-4 Improvement — Add concurrent mcp remove --force + rebuild race test under lifecycle lock

  • Location: test/mcp-bridge-destroy-marker-recovery.test.ts
  • Category: tests
  • Problem: Both mcp remove --force and rebuild acquire withMcpLifecycleLock. No test verifies lock serialization prevents marker clear race when both run concurrently.
  • Impact: Concurrent operations could cause the prepared marker to be cleared by one operation while the other still expects it, leaving inconsistent state.
  • Suggested action: Add a test that spawns two concurrent operations (one mcp remove --force, one rebuild) on a sandbox with prepared marker, and verifies only one succeeds with correct marker state.
  • Expected follow-up: Prefer a current-PR fix when local to changed code; defer only with rationale or linked follow-up.
  • Verification: Run: npx vitest run --project integration test/mcp-bridge-destroy-marker-recovery.test.ts -t 'concurrent'
  • Missing regression test: New test: 'concurrent mcp remove --force and rebuild serialize via lifecycle lock with correct marker outcome'
  • Done when: The local improvement is applied, or the PR notes why it should be deferred.
  • Evidence: Lifecycle lock exists but no regression test exercises concurrent access on a stuck-destroy sandbox.

Workflow run details

This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision.

@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — No blocking findings

Merge posture: No blocking advisor findings
Primary next action: Add or justify PRA-T1 and any related test follow-ups.
Open items: 0 required · 0 warnings · 0 suggestions · 5 test follow-ups
Since last review: 1 prior item resolved · 0 still apply · 0 new items found

Action checklist

  • PRA-T1 Add or justify test follow-up: Runtime validation
  • PRA-T2 Add or justify test follow-up: Runtime validation
  • PRA-T3 Add or justify test follow-up: Runtime validation
  • PRA-T4 Add or justify test follow-up: Runtime validation
  • PRA-T5 Add or justify test follow-up: Acceptance clause
Test follow-ups to resolve or justify

If these cover changed behavior, prefer adding them in this PR; otherwise state why existing coverage is enough or link the follow-up.

  • PRA-T1 Runtime validation — Live-like CLI validation: `mcp remove <server> --force` on a prepared-only provider-backed committed bridge clears `destroyPreparedAt` only after adapter removal, provider detach/delete, owned policy removal, and bridge manifest drain all succeed.. Static coverage is strong and directly exercises changed registry state-machine behavior, but the changed behavior spans real OpenShell gateway selection, in-sandbox adapter mutation, provider detach/delete, owned policy cleanup, registry persistence, CLI stderr/exit behavior, and rebuild preflight boundaries.
  • PRA-T2 Runtime validation — Live CLI validation: `rebuild` with a prepared-only MCP destroy marker prints the redacted recovery diagnostic and exits before backup/archive/delete starts.. Static coverage is strong and directly exercises changed registry state-machine behavior, but the changed behavior spans real OpenShell gateway selection, in-sandbox adapter mutation, provider detach/delete, owned policy cleanup, registry persistence, CLI stderr/exit behavior, and rebuild preflight boundaries.
  • PRA-T3 Runtime validation — Live-like validation: pending and both-marker `mcp remove --force` refuse and preserve markers plus cleanup manifest, then `destroy` completes idempotent provider/policy cleanup.. Static coverage is strong and directly exercises changed registry state-machine behavior, but the changed behavior spans real OpenShell gateway selection, in-sandbox adapter mutation, provider detach/delete, owned policy cleanup, registry persistence, CLI stderr/exit behavior, and rebuild preflight boundaries.
  • PRA-T4 Runtime validation — Live-like validation: a residual provider or policy cleanup failure during prepared-destroy recovery preserves `destroyPreparedAt` and the bridge manifest for retry.. Static coverage is strong and directly exercises changed registry state-machine behavior, but the changed behavior spans real OpenShell gateway selection, in-sandbox adapter mutation, provider detach/delete, owned policy cleanup, registry persistence, CLI stderr/exit behavior, and rebuild preflight boundaries.
  • PRA-T5 Acceptance clause — No deterministic linked issue clauses or issue comments were available in the validation context. — add test evidence or identify existing coverage. The validation context reports linkedIssues: [], so there were no trusted linked-issue clauses or issue comments to extract literally. PR body text was treated as untrusted orientation only.

Workflow run details

This is an automated, non-binding review; it still expects maintainers and agents to respond to each required or warning item. Treat suggestions as current-PR improvements when they touch changed code; defer only with maintainer rationale or a linked follow-up. A human maintainer must make the final merge decision.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/lib/actions/sandbox/rebuild-preflight-confirmation.ts`:
- Around line 56-58: The bail path in rebuild-preflight-confirmation currently
prints the raw failure message to stderr, bypassing the redaction used by the
existing log helper. Update the anonymous bail function so it applies the same
redact(message) handling before console.error, matching the behavior of log and
avoiding leaks from arbitrary errors bubbling through the rebuild pipeline.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 0201925b-1f31-46da-a0ed-de3585e80752

📥 Commits

Reviewing files that changed from the base of the PR and between 34ed8fd and 3ef4a70.

📒 Files selected for processing (5)
  • src/lib/actions/sandbox/mcp-bridge-remove.ts
  • src/lib/actions/sandbox/mcp-bridge-state.ts
  • src/lib/actions/sandbox/rebuild-preflight-confirmation.test.ts
  • src/lib/actions/sandbox/rebuild-preflight-confirmation.ts
  • test/mcp-bridge-destroy-marker-recovery.test.ts

Comment thread src/lib/actions/sandbox/rebuild-preflight-confirmation.ts
CI's ci-static-checks action runs `prek --stage pre-commit` in
non-modify mode; Biome format flagged two `vi.spyOn(...).mockImpl(...)`
chains and one `runNodeScript` parameter list on the branch for wrap
adjustments. Apply the formatter output verbatim (no test logic
changed; regression coverage still 15/15 across the two files).

Refs #6376

Signed-off-by: Yanyun Liao <yanyunl@nvidia.com>

@cv cv left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Three exact-head blockers remain: destroy markers are cleared before phase-specific cleanup/retry success is proven, so a failed recovery can lose its durable retry state; rebuild reaches the backup/delete path before checking those markers, so recovery is still too late; and the new bail path prints raw lower-level error text instead of passing it through the existing redaction boundary. Make marker clearing phase-aware, move the marker preflight ahead of destructive work, redact the surfaced error, and add failure/retry/redaction tests before rerunning CI and the Advisor.

…cted bail (#6376)

Address maintainer review (three exact-head blockers) on the #6376 fix:

- Phase-aware marker clearing + preserve retry state. `mcp remove --force`
  recovery now only clears the PREPARED (phase-one) marker — the sandbox still
  exists — and clears it only AFTER the removal succeeds, so a failed recovery
  preserves the durable retry marker (setBridgeState keeps it across the
  removal's own writes). The PENDING (phase-two) marker means OpenShell already
  deleted the sandbox; clearMcpDestroyMarkers and the guard now refuse it and
  point at `nemoclaw <name> destroy` instead of silently dropping the still-owed
  provider/policy cleanup. assertMcpDestroyNotPending gives phase-aware guidance.

- Move the marker preflight ahead of destructive work. rebuild ran the only MCP
  marker check inside the destroy phase, after the backup phase. Add
  assertMcpDestroyNotPending to runRebuildPreflightPhase, before backup/delete,
  so a stuck sandbox fails closed before any destructive work.

- Redact the surfaced bail error. createRebuildCommandContext's stderr bail now
  routes the message through the same `redact` boundary `log` uses, so a bailed
  rebuild cannot be the one path that leaks a URL/token.

Tests: phase-aware clear/refuse, --force recovery clears only after success,
failure preserves the prepared marker (retry state), rebuild preflight refuses
before backup, and bail-message redaction. Existing lifecycle test updated for
the phase-aware guard message.

Refs #6376

Signed-off-by: Yanyun Liao <yanyunl@nvidia.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/mcp-destroy-lifecycle.test.ts`:
- Around line 435-466: The two restore lifecycle tests are only checking that
GITHUB_TOKEN still exists, which allows the tests to pass even if the secret
value was changed. Update the assertions in the test cases around
restoreMcpBridgesAfterDestroyAbort and restoreMcpBridgesAfterRebuild to verify
process.env.GITHUB_TOKEN still equals the original
ambient-value-that-must-not-rotate after restore. Keep the existing presence
check if desired, but add a value assertion so the tests actually prove the
credential was not rotated.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: ada6cb20-d73d-4cb2-81cd-34f4fadb31a3

📥 Commits

Reviewing files that changed from the base of the PR and between 77e3738 and 6516d5b.

📒 Files selected for processing (7)
  • src/lib/actions/sandbox/mcp-bridge-remove.ts
  • src/lib/actions/sandbox/mcp-bridge-state.ts
  • src/lib/actions/sandbox/rebuild-preflight-confirmation.test.ts
  • src/lib/actions/sandbox/rebuild-preflight-confirmation.ts
  • src/lib/actions/sandbox/rebuild-preflight-phase.ts
  • test/mcp-bridge-destroy-marker-recovery.test.ts
  • test/mcp-destroy-lifecycle.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/lib/actions/sandbox/rebuild-preflight-confirmation.ts
  • src/lib/actions/sandbox/rebuild-preflight-confirmation.test.ts

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@test/mcp-destroy-lifecycle.test.ts`:
- Around line 435-466: The two restore lifecycle tests are only checking that
GITHUB_TOKEN still exists, which allows the tests to pass even if the secret
value was changed. Update the assertions in the test cases around
restoreMcpBridgesAfterDestroyAbort and restoreMcpBridgesAfterRebuild to verify
process.env.GITHUB_TOKEN still equals the original
ambient-value-that-must-not-rotate after restore. Keep the existing presence
check if desired, but add a value assertion so the tests actually prove the
credential was not rotated.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: ada6cb20-d73d-4cb2-81cd-34f4fadb31a3

📥 Commits

Reviewing files that changed from the base of the PR and between 77e3738 and 6516d5b.

📒 Files selected for processing (7)
  • src/lib/actions/sandbox/mcp-bridge-remove.ts
  • src/lib/actions/sandbox/mcp-bridge-state.ts
  • src/lib/actions/sandbox/rebuild-preflight-confirmation.test.ts
  • src/lib/actions/sandbox/rebuild-preflight-confirmation.ts
  • src/lib/actions/sandbox/rebuild-preflight-phase.ts
  • test/mcp-bridge-destroy-marker-recovery.test.ts
  • test/mcp-destroy-lifecycle.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/lib/actions/sandbox/rebuild-preflight-confirmation.ts
  • src/lib/actions/sandbox/rebuild-preflight-confirmation.test.ts
🛑 Comments failed to post (1)
test/mcp-destroy-lifecycle.test.ts (1)

435-466: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Test titles claim "without rotating" a secret/credential, but the assertions don't check the value.

Both tests set process.env.GITHUB_TOKEN = "ambient-value-that-must-not-rotate" to prove the ambient credential isn't rotated during restore, but:

  • Line 451 only checks Object.hasOwn(process.env, "GITHUB_TOKEN") — this passes even if the value were overwritten.
  • Lines 525-542 have no GITHUB_TOKEN assertion at all.

Neither test actually exercises its stated claim. As per path instructions, flag "conditionals that make a test pass without exercising its claim."

🧪 Proposed fix to assert the value, not just presence
-    expect(Object.hasOwn(process.env, "GITHUB_TOKEN")).toBe(true);
+    expect(process.env.GITHUB_TOKEN).toBe("ambient-value-that-must-not-rotate");

And for the second test (around line 536-541), add an equivalent assertion after restoreMcpBridgesAfterRebuild:

     await bridge.restoreMcpBridgesAfterRebuild("alpha", [bridgeEntries.github]);
 
+    expect(process.env.GITHUB_TOKEN).toBe("ambient-value-that-must-not-rotate");
     expect(testState.calls.some((call) => /^provider (create|update) /.test(call))).toBe(false);

Also applies to: 525-542

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@test/mcp-destroy-lifecycle.test.ts` around lines 435 - 466, The two restore
lifecycle tests are only checking that GITHUB_TOKEN still exists, which allows
the tests to pass even if the secret value was changed. Update the assertions in
the test cases around restoreMcpBridgesAfterDestroyAbort and
restoreMcpBridgesAfterRebuild to verify process.env.GITHUB_TOKEN still equals
the original ambient-value-that-must-not-rotate after restore. Keep the existing
presence check if desired, but add a value assertion so the tests actually prove
the credential was not rotated.

Source: Path instructions

The failure/retry case injected its failure through OpenShell provider
inspection, which behaves differently in CI (no live gateway) and crashed the
child process. Inject the failure deterministically instead: stub
ensureSandboxGatewaySelected to throw and use a provider-less bridge so the
removal fails at gateway selection — before any OpenShell work — and the
prepared marker must still be preserved.

Refs #6376

Signed-off-by: Yanyun Liao <yanyunl@nvidia.com>
Address Review Advisor required fixes on the #6376 recovery:

- PRA-2: removeMcpBridgeUnlocked now returns a discriminated outcome
  (removedTarget / cancelledPreparedAdd / markerOnlyNoEntries / noMatchingEntry
  / residualPreserved), and removeMcpBridge clears the phase-one destroy marker
  ONLY for a proven recovery. A wrong-server `--force` no-op (other entries
  remain) and an allowResidual residual cleanup no longer drop the durable
  retry marker.
- PRA-4: validateSandboxName at the top of clearMcpDestroyMarkers before any
  registry read/update.
- PRA-1/PRA-3: document the intentionally narrow product contract as a
  structured annotation (invalidState / sourceBoundary / sourceFixConstraint /
  regressionTest / removalCondition): prepared-only markers are recoverable with
  `mcp remove --force`; pending/both-marker state must finish `nemoclaw <name>
  destroy`.

Tests: add a wrong-server `--force` no-op case that must preserve the prepared
marker.

Refs #6376

Signed-off-by: Yanyun Liao <yanyunl@nvidia.com>
@wscurran wscurran added v0.0.77 area: cli Command line interface, flags, terminal UX, or output area: sandbox OpenShell sandbox lifecycle, runtime, config, or recovery bug-fix PR fixes a bug or regression platform: dgx-spark Affects DGX Spark hardware or workflows and removed v0.0.76 labels Jul 7, 2026
@apurvvkumaria apurvvkumaria self-assigned this Jul 7, 2026
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
@apurvvkumaria apurvvkumaria changed the title fix(rebuild,mcp): surface bail error + non-destructive MCP destroy recovery fix(rebuild,mcp): surface errors and recover prepared destroys Jul 7, 2026
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

E2E Target Results — ✅ All requested jobs passed

Run: 28900700830
Workflow ref: fix/mcp-destroy-marker-silent-rebuild-6376
Requested targets: (default — all supported)
Requested jobs: mcp-bridge,sandbox-rebuild
Summary: 2 passed, 0 failed, 0 cancelled, 0 skipped

Job Result
mcp-bridge ✅ success
sandbox-rebuild ✅ success

Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>

@cv cv left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed exact head d1881e0. The prior blockers are addressed: destroy-marker clearing is phase-aware and occurs only after a proven residual-free recovery with an empty bridge manifest; rebuild refuses markers in preflight before backup/deletion; surfaced bail diagnostics pass through the redaction boundary. Pending/both-marker state remains fail-closed and exact provider/policy ownership checks are preserved. Focused lifecycle, retry, residual, multi-bridge, credential-preservation, and redaction coverage passes; required CI and the trusted primary Advisor are green. The proposed focused live interrupted-marker and explicit concurrency regressions are worthwhile follow-ups but are non-blocking because the current paths share the lifecycle lock and required live MCP/rebuild lanes pass.

@ericksoa ericksoa added v0.0.78 and removed v0.0.77 labels Jul 8, 2026
@apurvvkumaria
apurvvkumaria merged commit 05ff005 into main Jul 8, 2026
43 checks passed
@apurvvkumaria
apurvvkumaria deleted the fix/mcp-destroy-marker-silent-rebuild-6376 branch July 8, 2026 18:38
@cjagwani cjagwani mentioned this pull request Jul 9, 2026
21 tasks
cv pushed a commit that referenced this pull request Jul 9, 2026
<!-- markdownlint-disable MD041 -->
## Summary

Prepares the user documentation for NemoClaw v0.0.78 by replacing the
unreleased section with release highlights and synchronizing the
affected inference, lifecycle, messaging, and CLI reference pages with
merged behavior.

## Changes

- Publish the v0.0.78 release-notes section with links to the most
specific user guides for each shipped behavior.
- Document authoritative Deep Agents route health, Nemotron Ultra
profile behavior, and Hermes compatible-endpoint context metadata.
- Document forced rebuild recovery after total backup failure and the
ownership-safe tunnel/full-stop behavior.
- Keep command examples and shared agent variants aligned with the
current OpenClaw, Hermes, and Deep Agents interfaces.

Source mapping:

- [#3787](#3787) ->
`docs/about/release-notes.mdx`: Record reliable workspace template
seeding during sandbox startup.
- [#4960](#4960) ->
`docs/about/release-notes.mdx`: Record safer detection of rewritten
OpenClaw gateway processes.
- [#5676](#5676) ->
`docs/about/release-notes.mdx`: Record warning-tolerant agent-list JSON
handling.
- [#5857](#5857) ->
`docs/about/release-notes.mdx`: Record synchronization of explicit
OpenClaw main-agent model state.
- [#5929](#5929) ->
`docs/about/release-notes.mdx`: Record copyable SSH port-forward
guidance for remote dashboards.
- [#6068](#6068) ->
`docs/about/release-notes.mdx`: Record custom-image plugin provenance
reconciliation.
- [#6116](#6116) ->
`docs/about/release-notes.mdx`: Record live-loopback dashboard-forward
recovery.
- [#6122](#6122) ->
`docs/about/release-notes.mdx`: Announce validated, round-trippable
policy YAML output.
- [#6211](#6211) ->
`docs/manage-sandboxes/lifecycle.mdx`, `docs/reference/commands.mdx`,
`docs/about/release-notes.mdx`: Explain the explicit no-backup `rebuild
--force` recovery boundary.
- [#6283](#6283) ->
`docs/about/release-notes.mdx`: Record Hermes WebUI port alignment.
- [#6293](#6293) ->
`docs/inference/switch-inference-providers.mdx`,
`docs/about/release-notes.mdx`: Document compatible-endpoint
context-length probing for Hermes.
- [#6320](#6320) ->
`docs/about/release-notes.mdx`: Record bounded gateway-recovery waits.
- [#6377](#6377) ->
`docs/reference/commands.mdx`, `docs/about/release-notes.mdx`: Explain
rebuild diagnostics and prepared MCP-destroy recovery.
- [#6412](#6412) ->
`docs/get-started/quickstart-langchain-deepagents-code.mdx`,
`docs/about/release-notes.mdx`: Document authoritative agent-visible
inference route health.
- [#6421](#6421) ->
`docs/about/release-notes.mdx`: Record the longer quiet-pull window for
managed vLLM images.
- [#6431](#6431) ->
`docs/inference/model-capability-audit.mdx`,
`docs/about/release-notes.mdx`: Document the version-pinned Nemotron
Ultra profile plugin.
- [#6439](#6439) ->
`docs/about/release-notes.mdx`: Summarize the authenticated, pinned
credential-capture helper boundary.
- [#6450](#6450) ->
`docs/manage-sandboxes/messaging-channels.mdx`,
`docs/reference/commands.mdx`, `docs/about/release-notes.mdx`: Document
host-forward cleanup and ownership-safe gateway-port release.
- [#6474](#6474) ->
`docs/manage-sandboxes/messaging-channels.mdx`,
`docs/about/release-notes.mdx`: Record composable OpenClaw messaging
runtime loaders.
- [#6475](#6475) ->
`docs/about/release-notes.mdx`: Record removal of the unavailable Kimi
K2.6 production endpoint option.
- [#6480](#6480) ->
`docs/about/release-notes.mdx`: Record stderr routing for the plugin
registration banner.
- [#6481](#6481) ->
`docs/about/release-notes.mdx`: Record post-pull Ollama model discovery
checks.
- [#6482](#6482) ->
`docs/about/release-notes.mdx`: Record Ollama model warm-up after daemon
restart.
- [#6486](#6486) ->
`docs/about/release-notes.mdx`: Publish the opt-in, thread-scoped Deep
Agents auto-approval boundary.
- [#6490](#6490) ->
`docs/about/release-notes.mdx`: Record diagnostics for custom images
missing the managed runtime.
- [#6494](#6494) ->
`docs/inference/model-capability-audit.mdx`,
`docs/about/release-notes.mdx`: Document nonempty tool-call content
preservation and placeholder rejection.
- [#6497](#6497) ->
`docs/get-started/quickstart-langchain-deepagents-code.mdx`,
`docs/about/release-notes.mdx`: Document isolated Deep Agents
route-probe output.
- [#6506](#6506) ->
`docs/get-started/quickstart-langchain-deepagents-code.mdx`,
`docs/about/release-notes.mdx`: Document observability-preserving
managed route probes.
- [#6508](#6508) ->
`docs/about/release-notes.mdx`: Link the new extension taxonomy and
SDK-readiness reference from the release summary.

Release-source verification: GitHub reports all 29 cited source PRs as
merged with base `main`, and every merge commit is an ancestor of
`origin/main` at `17bf9a6a9688b3b1d69cf4b37d3f23110acb055e`. No
source-mapping mismatches were found.

## Type of Change

- [ ] Code change (feature, bug fix, or refactor)
- [ ] Code change with doc updates
- [ ] Doc only (prose changes, no code sample modifications)
- [x] Doc only (includes code sample changes)

## Quality Gates

<!-- Check exactly one tests line and one docs line. Check other lines
when applicable. Add every requested justification or approval
reference. -->
- [ ] Tests added or updated for changed behavior
- [ ] Existing tests cover changed behavior — justification:
- [x] Tests not applicable — justification: Documentation-only
release-prep changes; `npm run docs` validates variants, routes, and
Fern content.
- [x] Docs updated for user-facing behavior changes
- [ ] Docs not applicable — justification:
- [ ] Sensitive paths changed (security, policy, credentials, preflight,
onboarding, inference, runner, sandbox, or messaging)
- [ ] Sensitive-path review completed or maintainer-approved waiver
recorded — reviewer/approval link/justification:
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## Verification

<!-- Check each applicable item only when supported by the requested
evidence. Run targeted tests once per relevant change set and rerun
after later edits or hook autofixes that can affect the tested behavior.
Do not rerun hook-covered checks. -->
- [x] PR description includes the DCO sign-off declaration and every
commit appears as `Verified` in GitHub
- [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or
`npm run check:diff` passed when hooks were skipped or unavailable
- [x] Targeted behavior tests pass for the current change set, or tests
are marked not applicable above — command/result or justification: Tests
are not applicable to this documentation-only change set.
- [ ] Applicable broad gate passed — `npm test` for broad
runtime/test-harness changes; `npm run check` for repo-wide
validation/coverage changes — command/result:
- [x] Quality Gates section completed with required justifications or
waivers
- [x] No secrets, API keys, or credentials committed
- [ ] `npm run docs` builds without warnings (doc changes only) — exited
0 with zero errors; Fern reported the existing unauthenticated
redirect-check and light-mode contrast warnings.
- [x] Doc pages follow the [style
guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md)
(doc changes only)
- [ ] New doc pages include SPDX header and frontmatter (new pages only)

---
<!-- DCO sign-off is required in this PR description, and every commit
must appear as Verified in GitHub. Run: git config user.name && git
config user.email -->
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>

---------

Signed-off-by: cjagwani <cjagwani@nvidia.com>
Hadar301 pushed a commit to Hadar301/NemoClaw-OpenShift that referenced this pull request Jul 12, 2026
…A#6377)

<!-- markdownlint-disable MD041 -->
## Summary

This PR makes rebuild surface redacted MCP destroy diagnostics before
backup or deletion and adds a non-destructive recovery path for
prepared-only destroy transactions.

It deliberately keeps pending or both-marker transactions fail closed
because `destroyPendingAt` records confirmed OpenShell deletion while
provider or policy cleanup may still be owed.

## Related Issue

Addresses NVIDIA#6376.

The issue's literal reproduction manually injects both markers into a
live sandbox. That is not a valid lifecycle transition. When
`destroyPendingAt` is present, pending takes precedence and recovery
must finish through `nemoclaw <name> destroy` rather than erase durable
retry state.

## Recovery Contract

| Registry state | Meaning | Recovery |
| --- | --- | --- |
| No destroy marker | No durable destroy transaction is active. | Normal
MCP commands and rebuild proceed. |
| `destroyPreparedAt` only | Adapter scrub and provider detach
completed, but sandbox deletion is not durably confirmed. | If the
sandbox is still live, run `nemoclaw <name> mcp remove <server> --force`
for each registered server. The marker clears only after residual-free
cleanup drains every bridge entry. |
| `destroyPendingAt`, with or without `destroyPreparedAt` | The registry
records confirmed OpenShell deletion and retained cleanup may still be
owed. | `mcp remove --force` refuses and preserves both markers and the
cleanup manifest. Run `nemoclaw <name> destroy` to finish idempotent
cleanup. |

## Changes

- Print non-throw rebuild bail messages on stderr through the existing
redaction boundary.
- Check MCP destroy markers during rebuild preflight, before target
preflights, backup, or deletion.
- Clear a prepared marker only after the requested removal succeeds
without residuals and no bridge entries remain.
- Preserve markers and manifests after failures, wrong-server no-ops,
tolerated residuals, partial multi-bridge cleanup, and pending or
both-marker refusal.
- Assert that destroy-abort and rebuild restoration do not rotate the
ambient host credential value.
- Document the phase-aware recovery contract in the MCP guide and
command references.

## Type of Change

- [ ] Code change (feature, bug fix, or refactor)
- [x] Code change with doc updates
- [ ] Doc only (prose changes, no code sample modifications)
- [ ] Doc only (includes code sample changes)

## Quality Gates

- [x] Tests added or updated for changed behavior
- [ ] Existing tests cover changed behavior — justification:
- [ ] Tests not applicable — justification:
- [x] Docs updated for user-facing behavior changes
- [ ] Docs not applicable — justification:
- [x] Sensitive paths changed (security, policy, credentials, preflight,
onboarding, inference, runner, sandbox, or messaging)
- [x] Sensitive-path review completed or maintainer-approved waiver
recorded — marker ordering, pending cleanup ownership, failure
preservation, and the prepared-only recovery boundary were reviewed
against the durable state transitions and locked with regression tests.
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## Verification

- [x] PR description includes the DCO sign-off declaration and every
commit appears as `Verified` in GitHub
- [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed
- [x] Targeted behavior tests pass for the current change set:
- `npx vitest run --project integration
test/mcp-bridge-destroy-marker-recovery.test.ts
test/mcp-destroy-lifecycle.test.ts` — 32/32
- `npx vitest run --project cli
src/lib/actions/sandbox/mcp-bridge-*.test.ts` — 106/106
- [ ] Applicable broad gate passed — not run because the change is
scoped to MCP lifecycle behavior; the full MCP CLI scope and focused
integration suite passed.
- [x] Quality Gates section completed with required justifications
- [x] No secrets, API keys, or credentials committed
- [ ] `npm run docs` builds without warnings — completed with 0 errors
and 2 existing Fern warnings
- [x] Doc pages follow the [style
guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md)
- [ ] New doc pages include SPDX header and frontmatter — no new pages

Additional checks:

- `npm run typecheck:cli`
- `npm run test:titles:check`
- `npm run test-size:check`
- Biome check on all touched TypeScript and test files
- Generated agent-variant documentation consistency check

## AI Disclosure

- [x] AI-assisted — tools: Claude Code and Codex

---
Signed-off-by: Yanyun Liao <yanyunl@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>

---------

Signed-off-by: Yanyun Liao <yanyunl@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Co-authored-by: Apurv Kumaria <akumaria@nvidia.com>
Hadar301 pushed a commit to Hadar301/NemoClaw-OpenShift that referenced this pull request Jul 12, 2026
<!-- markdownlint-disable MD041 -->
## Summary

Prepares the user documentation for NemoClaw v0.0.78 by replacing the
unreleased section with release highlights and synchronizing the
affected inference, lifecycle, messaging, and CLI reference pages with
merged behavior.

## Changes

- Publish the v0.0.78 release-notes section with links to the most
specific user guides for each shipped behavior.
- Document authoritative Deep Agents route health, Nemotron Ultra
profile behavior, and Hermes compatible-endpoint context metadata.
- Document forced rebuild recovery after total backup failure and the
ownership-safe tunnel/full-stop behavior.
- Keep command examples and shared agent variants aligned with the
current OpenClaw, Hermes, and Deep Agents interfaces.

Source mapping:

- [NVIDIA#3787](NVIDIA#3787) ->
`docs/about/release-notes.mdx`: Record reliable workspace template
seeding during sandbox startup.
- [NVIDIA#4960](NVIDIA#4960) ->
`docs/about/release-notes.mdx`: Record safer detection of rewritten
OpenClaw gateway processes.
- [NVIDIA#5676](NVIDIA#5676) ->
`docs/about/release-notes.mdx`: Record warning-tolerant agent-list JSON
handling.
- [NVIDIA#5857](NVIDIA#5857) ->
`docs/about/release-notes.mdx`: Record synchronization of explicit
OpenClaw main-agent model state.
- [NVIDIA#5929](NVIDIA#5929) ->
`docs/about/release-notes.mdx`: Record copyable SSH port-forward
guidance for remote dashboards.
- [NVIDIA#6068](NVIDIA#6068) ->
`docs/about/release-notes.mdx`: Record custom-image plugin provenance
reconciliation.
- [NVIDIA#6116](NVIDIA#6116) ->
`docs/about/release-notes.mdx`: Record live-loopback dashboard-forward
recovery.
- [NVIDIA#6122](NVIDIA#6122) ->
`docs/about/release-notes.mdx`: Announce validated, round-trippable
policy YAML output.
- [NVIDIA#6211](NVIDIA#6211) ->
`docs/manage-sandboxes/lifecycle.mdx`, `docs/reference/commands.mdx`,
`docs/about/release-notes.mdx`: Explain the explicit no-backup `rebuild
--force` recovery boundary.
- [NVIDIA#6283](NVIDIA#6283) ->
`docs/about/release-notes.mdx`: Record Hermes WebUI port alignment.
- [NVIDIA#6293](NVIDIA#6293) ->
`docs/inference/switch-inference-providers.mdx`,
`docs/about/release-notes.mdx`: Document compatible-endpoint
context-length probing for Hermes.
- [NVIDIA#6320](NVIDIA#6320) ->
`docs/about/release-notes.mdx`: Record bounded gateway-recovery waits.
- [NVIDIA#6377](NVIDIA#6377) ->
`docs/reference/commands.mdx`, `docs/about/release-notes.mdx`: Explain
rebuild diagnostics and prepared MCP-destroy recovery.
- [NVIDIA#6412](NVIDIA#6412) ->
`docs/get-started/quickstart-langchain-deepagents-code.mdx`,
`docs/about/release-notes.mdx`: Document authoritative agent-visible
inference route health.
- [NVIDIA#6421](NVIDIA#6421) ->
`docs/about/release-notes.mdx`: Record the longer quiet-pull window for
managed vLLM images.
- [NVIDIA#6431](NVIDIA#6431) ->
`docs/inference/model-capability-audit.mdx`,
`docs/about/release-notes.mdx`: Document the version-pinned Nemotron
Ultra profile plugin.
- [NVIDIA#6439](NVIDIA#6439) ->
`docs/about/release-notes.mdx`: Summarize the authenticated, pinned
credential-capture helper boundary.
- [NVIDIA#6450](NVIDIA#6450) ->
`docs/manage-sandboxes/messaging-channels.mdx`,
`docs/reference/commands.mdx`, `docs/about/release-notes.mdx`: Document
host-forward cleanup and ownership-safe gateway-port release.
- [NVIDIA#6474](NVIDIA#6474) ->
`docs/manage-sandboxes/messaging-channels.mdx`,
`docs/about/release-notes.mdx`: Record composable OpenClaw messaging
runtime loaders.
- [NVIDIA#6475](NVIDIA#6475) ->
`docs/about/release-notes.mdx`: Record removal of the unavailable Kimi
K2.6 production endpoint option.
- [NVIDIA#6480](NVIDIA#6480) ->
`docs/about/release-notes.mdx`: Record stderr routing for the plugin
registration banner.
- [NVIDIA#6481](NVIDIA#6481) ->
`docs/about/release-notes.mdx`: Record post-pull Ollama model discovery
checks.
- [NVIDIA#6482](NVIDIA#6482) ->
`docs/about/release-notes.mdx`: Record Ollama model warm-up after daemon
restart.
- [NVIDIA#6486](NVIDIA#6486) ->
`docs/about/release-notes.mdx`: Publish the opt-in, thread-scoped Deep
Agents auto-approval boundary.
- [NVIDIA#6490](NVIDIA#6490) ->
`docs/about/release-notes.mdx`: Record diagnostics for custom images
missing the managed runtime.
- [NVIDIA#6494](NVIDIA#6494) ->
`docs/inference/model-capability-audit.mdx`,
`docs/about/release-notes.mdx`: Document nonempty tool-call content
preservation and placeholder rejection.
- [NVIDIA#6497](NVIDIA#6497) ->
`docs/get-started/quickstart-langchain-deepagents-code.mdx`,
`docs/about/release-notes.mdx`: Document isolated Deep Agents
route-probe output.
- [NVIDIA#6506](NVIDIA#6506) ->
`docs/get-started/quickstart-langchain-deepagents-code.mdx`,
`docs/about/release-notes.mdx`: Document observability-preserving
managed route probes.
- [NVIDIA#6508](NVIDIA#6508) ->
`docs/about/release-notes.mdx`: Link the new extension taxonomy and
SDK-readiness reference from the release summary.

Release-source verification: GitHub reports all 29 cited source PRs as
merged with base `main`, and every merge commit is an ancestor of
`origin/main` at `17bf9a6a9688b3b1d69cf4b37d3f23110acb055e`. No
source-mapping mismatches were found.

## Type of Change

- [ ] Code change (feature, bug fix, or refactor)
- [ ] Code change with doc updates
- [ ] Doc only (prose changes, no code sample modifications)
- [x] Doc only (includes code sample changes)

## Quality Gates

<!-- Check exactly one tests line and one docs line. Check other lines
when applicable. Add every requested justification or approval
reference. -->
- [ ] Tests added or updated for changed behavior
- [ ] Existing tests cover changed behavior — justification:
- [x] Tests not applicable — justification: Documentation-only
release-prep changes; `npm run docs` validates variants, routes, and
Fern content.
- [x] Docs updated for user-facing behavior changes
- [ ] Docs not applicable — justification:
- [ ] Sensitive paths changed (security, policy, credentials, preflight,
onboarding, inference, runner, sandbox, or messaging)
- [ ] Sensitive-path review completed or maintainer-approved waiver
recorded — reviewer/approval link/justification:
- [ ] Non-success, skipped, or missing CI check accepted by maintainer —
check name, approval link, and follow-up issue:

## Verification

<!-- Check each applicable item only when supported by the requested
evidence. Run targeted tests once per relevant change set and rerun
after later edits or hook autofixes that can affect the tested behavior.
Do not rerun hook-covered checks. -->
- [x] PR description includes the DCO sign-off declaration and every
commit appears as `Verified` in GitHub
- [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or
`npm run check:diff` passed when hooks were skipped or unavailable
- [x] Targeted behavior tests pass for the current change set, or tests
are marked not applicable above — command/result or justification: Tests
are not applicable to this documentation-only change set.
- [ ] Applicable broad gate passed — `npm test` for broad
runtime/test-harness changes; `npm run check` for repo-wide
validation/coverage changes — command/result:
- [x] Quality Gates section completed with required justifications or
waivers
- [x] No secrets, API keys, or credentials committed
- [ ] `npm run docs` builds without warnings (doc changes only) — exited
0 with zero errors; Fern reported the existing unauthenticated
redirect-check and light-mode contrast warnings.
- [x] Doc pages follow the [style
guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md)
(doc changes only)
- [ ] New doc pages include SPDX header and frontmatter (new pages only)

---
<!-- DCO sign-off is required in this PR description, and every commit
must appear as Verified in GitHub. Run: git config user.name && git
config user.email -->
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>

---------

Signed-off-by: cjagwani <cjagwani@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: cli Command line interface, flags, terminal UX, or output area: sandbox OpenShell sandbox lifecycle, runtime, config, or recovery bug-fix PR fixes a bug or regression platform: dgx-spark Affects DGX Spark hardware or workflows

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants